Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

• maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.

• maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.

This is an automated suggestion to help route contributions to the appropriate branch.

Thanks for doing your first INAV PR! A lot of people will be very happy about this, because they've been asking for it for a long time. This is great, if it works out.

You mentioned comments. Let me comment on comments. :)
We have this code:

if(isEmpty(cmdline))
{
    cliPrint(parseErrString);
    return;
}

That's great code - totally self-explanatory. Someone who isn't even a programmer could likely read and pretty much understand that because it reads pretty much like English prose. If cmdLine is empty, print an error and return. Awesome.

THEREFORE, the comment repeating what the code says may be unnecessary noise that actually ends up making the code harder to read by making it longer:

if(isEmpty(cmdline))
{
    // Nothing sent, so fail and bail.
    cliPrint(parseErrString);
    return;
}

There's also the question there of -- are the comments before the code or after? Hmm, both?
Typically if a comment was needed, we'd put it before the code is is explaining:

    // Nothing sent, so fail and bail.
    if(isEmpty(cmdline))
    {
        cliPrint(parseErrString);
        return;
    }

But here there is nothing to explain - it's great, self-evident code.

Typically, comments should describe WHY the code is doing something that isn't obvious.
It would answer a question the reader might have - "why did they do that instead of just doing ...?"
This doesn't gain from a comment:

      sum = sum + newValue;

This certainly might!

        sum = sum + 0.0;

A comment could say 0.0 is added to make it a float. ( Though maybe explicitly casting to a float would be better in that example.)

We have a function called constructPowerTable(). Which contructs the power table, of course.
The comment "// Construct the power table" is just a distraction because the function is literally called constructPowerTable.

Thanks for doing your first INAV PR! A lot of people will be very happy about this, because they've been asking for it for a long time. This is great, if it works out.

You mentioned comments. Let me comment on comments. :) We have this code:

if(isEmpty(cmdline))
{
    cliPrint(parseErrString);
    return;
}

That's great code - totally self-explanatory. Someone who isn't even a programmer could likely read and pretty much understand that because it reads pretty much like English prose. If cmdLine is empty, print an error and return. Awesome.

THEREFORE, the comment repeating what the code says may be unnecessary noise that actually ends up making the code harder to read by making it longer:

if(isEmpty(cmdline))
{
    // Nothing sent, so fail and bail.
    cliPrint(parseErrString);
    return;
}

There's also the question there of -- are the comments before the code or after? Hmm, both? Typically if a comment was needed, we'd put it before the code is is explaining:

    // Nothing sent, so fail and bail.
    if(isEmpty(cmdline))
    {
        cliPrint(parseErrString);
        return;
    }

But here there is nothing to explain - it's great, self-evident code.

Typically, comments should describe WHY the code is doing something that isn't obvious. It would answer a question the reader might have - "why did they do that instead of just doing ...?" This doesn't gain from a comment:

      sum = sum + newValue;

This certainly might!

        sum = sum + 0.0;

A comment could say 0.0 is added to make it a float. ( Though maybe explicitly casting to a float would be better in that example.)

We have a function called constructPowerTable(). Which contructs the power table, of course. The comment "// Construct the power table" is just a distraction because the function is literally called constructPowerTable.

This all sounds good to me. Took a pass through and cleaned up the comments.

Additionally, this commit should fix the unused static function errros from SITL builds. Had the commands in preprocessor conditionals but not the static functions used to service those commands. The functions should now follow the command definitions

Hi @SomeTestStuffDoesntWork this is a great addition and something I've been looking for in INAV since I have some IRC Tramp based VTXs that won't work properly in the current implementation. I'm in the process of testing your code applied to INAV 9.0.1 and came across some problems as I wasn't fully able to replicate the success you had in your PR.

I was able to fix the first problem (though not sure if it is correct), which is that default and min values for the new cli variable vtx_pw_override_mw fails the system checks in INAV configurator. To fix this, in settings.yaml, I changed the type to unit16_t, default_value to 0, and min to 0. Additionally, when declaring the vtxTrampOverride struct in vtx_tramp.h, the struct's name seems to be missing so I changed it to:

typedef struct vtxTrampPwOverride {
    int vtxPwOverrideMw;
} vtxTrampPwOverride_t;

I've confirmed with these two changes that the firmware builds correctly and does not throw any system errors in INAV Configurator.

For reference, the FC that I'm testing this on is the AtomRC F405 Navi and the Jhemcu Ruibet Tran 3016 VTX.

After doing a full erase flash with the modified firmware, I applied the VTX power table using tramp_pl_table because I don't care about the default table. I use the cli command: tramp_pl_table 25 200 400 800 1600 and the value sets as expected. I then set the power value in the Configuration tab like I normally would for any other VTX to test that all the power levels work. For some strange reason, I'm able to confirm that power levels 1, 2, and 3 that pertain to 25mw, 200mw, and 400mw work as expected (confirmed through the red LED flashes on the VTX). However, it doesn't work for power level 4 (800mw) or 5 (1600mw). When I query what the config is doing with tramp_config_dump, it appears that for power level 4, the FC is sending 600mw to the VTX.

I hardcoded the table the old way as described in this discussion #8340 and was able to confirm all 5 power values work as expected once I force INAV to use the hardcoded table for the 1600mw VTX using the set vtx_max_power_override = 1600 command.

Would you happen to know why this could be?

Additionally, I don't fully understand the purpose of the variable vtxPwOverrideMw that you create. It seems that once I define my custom table, the value of vtxPwOverrideMw remains at 25. Also, is there a conflict when both vtxPwOverrideMw and vtx_max_power_override are used? Is one supposed to superscede the other?

I think your implementation could also be refined if there is no default table at all. To simplify the code and usability, would it make sense to have the user manually define the power values for their specific IRC Tramp vtx that way there's little to no confusions about what's overriding what?

Thanks for your help with helping me understand your code better!